Skip to content

fix: integer overflow in calculation#15

Merged
echobt merged 1 commit intoPlatformNetwork:mainfrom
eureka0928:fix/int-overflow
Jan 8, 2026
Merged

fix: integer overflow in calculation#15
echobt merged 1 commit intoPlatformNetwork:mainfrom
eureka0928:fix/int-overflow

Conversation

@eureka0928
Copy link
Copy Markdown
Contributor

@eureka0928 eureka0928 commented Jan 8, 2026

Summary

Integer Overflow in Stake Consensus Calculation

Severity: Critical
File: crates/consensus/src/stake_weighted_pbft.rs:121,130
Effort: Small (5-10 lines)

// Current - can overflow when approve_stake > u64::MAX / 2
approve_stake * 2 > total_stake

Impact: When stake exceeds ~9.2e18, multiplication wraps around causing:

  • Consensus incorrectly accepted/rejected
  • Potential attack vector for manipulating outcomes

Fix: Reorder to avoid multiplication:

approve_stake > total_stake / 2

Summary by CodeRabbit

  • Bug Fixes
    • Resolved potential arithmetic overflow in consensus stake threshold calculations. The fix ensures stable operation with large stake values while preserving consensus threshold behavior and logic.

✏️ Tip: You can customize this high-level summary in your review settings.

Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=42954461

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 8, 2026

📝 Walkthrough

Walkthrough

Modified stake comparison logic in the PBFT consensus module, replacing multiplication-based threshold checks with division-based checks to prevent integer overflow when evaluating stake consensus and rejection conditions across two functions.

Changes

Cohort / File(s) Summary
Overflow-Safe Stake Threshold Comparison
crates/consensus/src/stake_weighted_pbft.rs
Replaced stake * 2 > total_stake comparisons with stake > total_stake / 2 in has_stake_consensus() and is_stake_rejected() to mitigate overflow risk with large stake values while maintaining 50%+ threshold semantics

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Two functions checked, two times refined,
From multiply to divide, safer logic aligned,
No overflow shall haunt the stakes so high,
Just cleaner math beneath the consensus sky!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: integer overflow in calculation' is directly related to the main change, which replaces overflow-prone multiplication-based stake threshold comparisons with division-based checks to prevent integer overflow.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0cd0460 and 534c7f9.

📒 Files selected for processing (1)
  • crates/consensus/src/stake_weighted_pbft.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/consensus/src/stake_weighted_pbft.rs (1)
crates/consensus/src/stake_governance.rs (1)
  • total_stake (369-372)
🔇 Additional comments (2)
crates/consensus/src/stake_weighted_pbft.rs (2)

121-122: LGTM! Critical overflow fix correctly implemented.

The division-based approach prevents integer overflow when approve_stake > u64::MAX / 2 while preserving the strictly-greater-than-50% consensus threshold semantics. Division by zero is already guarded at lines 116-118.


131-132: LGTM! Rejection threshold overflow fix correctly implemented.

The division-based approach prevents integer overflow while maintaining the >50% rejection threshold. Division by zero is already guarded at lines 127-129.

Consider adding a specific test case to verify behavior with large stake values near the overflow boundary:

#[test]
fn test_overflow_prevention_large_stakes() {
    let kp = Keypair::generate();
    let proposal = Proposal::new(
        ProposalAction::NewBlock { state_hash: [0u8; 32] },
        kp.hotkey(),
        0,
    );
    
    let mut round = StakeWeightedRoundState::new(proposal, 30);
    
    // Test with stake value near u64::MAX / 2
    let large_stake = u64::MAX / 2 + 1000;
    let total_stake = u64::MAX - 1000;
    
    let voter = Keypair::generate();
    round.add_vote(
        Vote::approve(round.proposal.id, voter.hotkey()),
        Stake::new(large_stake),
    );
    
    // Should not panic and correctly evaluate consensus
    assert!(round.has_stake_consensus(total_stake));
}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@echobt echobt merged commit 138fbf9 into PlatformNetwork:main Jan 8, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants